Gamm Spec Out V3 - Modulize File Structure #541
Conversation
mattverse
changed the title
|
Why is this PR against main instead of the v2 branch |
|
my bad - should be v2 branch |
x/gamm/pool-models/balancer/msgs.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func ValidateFutureGovernor(governor string) error { |
There was a problem hiding this comment.
This should be defined in the pool interface folder I think
There was a problem hiding this comment.
What's the pool interface folder? Did you mean types.go where poolI used to be defined?
types.go was deleted, so moved the method to balancer_pool.go for now!
| @@ -1,5 +1,5 @@ | |||
| syntax = "proto3"; | |||
| package osmosis.gamm.v1beta1; | |||
| package osmosis.gamm.poolmodels; | |||
There was a problem hiding this comment.
should this still be behind a v1beta1?
There was a problem hiding this comment.
Are you referring to the entire file itself? If my understanding is right upon your question, pool-models is declared as a whole new package in gamm, thats where this is coming from.
There was a problem hiding this comment.
I see, tbh I don't really understand the v1beta1 stuff. Lets make an issue for this question, and then ask regen folks post-holidays
There was a problem hiding this comment.
haha. I'm supposing its just a legacy in structuring the proto files from the cosmos-sdk and that it does not have any additional functionallities. I didn't have any issues when I wasn't using v1beta1 as a package file structure, they should be good to be removed as well
|
@ValarDragon Can you also review the |
Codecov Report
@@ Coverage Diff @@
## main #541 +/- ##
==========================================
- Coverage 18.39% 18.14% -0.25%
==========================================
Files 173 178 +5
Lines 24321 24453 +132
==========================================
- Hits 4473 4437 -36
- Misses 19086 19257 +171
+ Partials 762 759 -3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
LGTM.
I think we just need to make followup issues for:
- Separating out general pool parameters into their own struct. (Swap fee, Exit fee)
- Removing PoolAsset as a struct, and moving all weight handling to be internal to balancer
- the .proto
v1beta1question.
Then I think we're good to merge this!
| @@ -1,5 +1,5 @@ | |||
| syntax = "proto3"; | |||
| package osmosis.gamm.v1beta1; | |||
| package osmosis.gamm.poolmodels; | |||
There was a problem hiding this comment.
I see, tbh I don't really understand the v1beta1 stuff. Lets make an issue for this question, and then ask regen folks post-holidays
|
|
||
| // assumes 0 < d < 1 | ||
| func poolAssetsMulDec(base []PoolAsset, d sdk.Dec) []PoolAsset { |
There was a problem hiding this comment.
Do we just no longer need these pool asset arithmetic methods?
There was a problem hiding this comment.
this was moved to balancer_pool.go bc the concept of pool weight moved to balancer package
|
Just created three issues following this PR |
|
@ValarDragon Could you also review |
|
Agreed that a lot of these methods should be changed. I think it'd be easiest to do that when separating out the parameters. (So we can remove all the weight references, etc.) I think we also may want secondary interfaces for different types of pools. (CFMM, batched-CFMM, etc.) but I think it's perhaps too early for doing that rn / unclear if we want to push that to cosmwasm. |
Description
This PR is the 3rd version of Gamm spec out, including the main generalization and spec out of gamm module by seperating pool-models/balancer package with types package. This PR includes the following changes in detail:
proto/osmosis/gamm/v1beta,proto/osmosis/gamm/pool-models/balancerwhere gamm/v1beta directory includes protobufs that could be used for generalized usage(such asPoolAsset), and gamm/pool-models/balancer directory only includes protobufs limited to the scope of a balancer pool.proto/osmosis/gamm/pool-models/balanceralso includes tx.proto, which includes Msgs that uses protos defined in the package.A package defined under pool-models directory(e.g balancer package in this PR) should meet the following requirements.
poolIfor the specific poolIn the cases of where tx or msgs that has dependency on the created pool model package, the following requirements has to be specified.
RegisterLegacyAminoCodec,RegisterInterfacesrespectively.Even though all pools would have their own poolParams proto, query for PoolParams still exists in generalized form(in the form of Any). Thereby,
grpc_query.goshould be implementing poolParams type of the specific module using switch cases.For contributor use:
docs/) or specification (x/<module>/spec/)Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorer